Skip to content

feat(dfx-orbit,wallet): support --wasm-memory-persistence for external canister upgrades#641

Open
aterga wants to merge 7 commits into
mainfrom
claude/wasm-memory-persistence-orbit-7akm1u
Open

feat(dfx-orbit,wallet): support --wasm-memory-persistence for external canister upgrades#641
aterga wants to merge 7 commits into
mainfrom
claude/wasm-memory-persistence-orbit-7akm1u

Conversation

@aterga

@aterga aterga commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

The station already accepts wasm_memory_persistence and skip_pre_upgrade on external-canister upgrades (via CanisterUpgradeOptionsInput, added in #634), but neither of the two places users actually trigger an upgrade exposed them. This PR wires those upgrade options through the dfx-orbit CLI and the wallet Install screen.

Without wasm_memory_persistence = keep, Motoko canisters that use Enhanced Orthogonal Persistence cannot be safely upgraded through Orbit — the IC defaults to replace, wiping their main memory.

What changed

dfx-orbit CLI (tools/dfx-orbit)

  • New flags on request canister install / verify canister install:
    • --wasm-memory-persistence keep|replace
    • --skip-pre-upgrade
  • The flags are folded into CanisterInstallMode::Upgrade(Some(..)) and are rejected for install/reinstall (the IC only honours them on upgrades).
  • verify now compares the full install mode (including the upgrade options), so a request whose stored persistence mode differs from what the verifier expects is rejected instead of silently passing.
  • The change-canister request display now prints the upgrade options.
  • README documents the new flags.

Wallet Install screen (apps/wallet)

  • New "Wasm Memory Persistence" select (Default / Keep / Replace) and a "Skip pre-upgrade hook" toggle that appear only when the install mode is upgrade, wired into the install model's mode.
  • When neither option is set the request collapses back to a plain { upgrade: [] }, keeping it identical to the previous behaviour.
  • New CanisterWasmMemoryPersistenceSelect input component and locale strings (en/fr/pt).

API

  • Derive PartialEq/Eq for station_api::CanisterInstallMode so the CLI can compare the full mode during verification (additive; does not change the candid interface).

Tests

  • dfx-orbit unit tests: install_mode() folds each flag correctly, produces Upgrade(None) when unset, and rejects the flags for install/reinstall with the exact error message.
  • Integration test: an upgrade request carrying wasm_memory_persistence = replace + skip_pre_upgrade round-trips through the station; verify accepts a matching request and rejects a mismatched persistence mode; the approved upgrade then executes, proving the options are forwarded to the management canister. (keep cannot execute against the non-EOP test wasm — the IC rejects it for modules without the EOP metadata section.)
  • Wallet component tests: the upgrade options are hidden unless the mode is upgrade, both options fold into mode, clearing them collapses back to a plain upgrade, and the persistence select's option↔candid-union mapping is covered by a dedicated spec.

Verification performed here

  • wallet vue-tsc --noEmit type-check: clean.
  • wallet vitest for the touched components/inputs: passing.
  • prettier --check and eslint on the changed wallet files: clean.
  • rustfmt --check on the changed Rust files: clean.
  • ⚠️ A full cargo build/cargo test/clippy could not run in this environment because the dfinity/response-verification git dependency is outside the sandbox's egress allow-list. CI exercises the Rust build, unit tests, and PocketIC integration tests.

🤖 Generated with Claude Code

claude added 3 commits July 1, 2026 14:53
…l canister upgrades

Expose the upgrade options that the station already accepts
(`CanisterUpgradeOptionsInput`) in the two places users trigger external
canister upgrades: the dfx-orbit CLI and the wallet Install screen.

dfx-orbit CLI:
- Add `--wasm-memory-persistence <keep|replace>` and `--skip-pre-upgrade`
  flags to `request/verify canister install`. The flags are folded into
  `CanisterInstallMode::Upgrade(Some(..))` and rejected for
  install/reinstall.
- `verify` now compares the full install mode (including the upgrade
  options), so a request with a mismatched persistence mode is rejected.
- Show the upgrade options in the change-canister request display.

Wallet Install screen:
- Add a "Wasm Memory Persistence" select and a "Skip pre-upgrade hook"
  toggle that appear only for the upgrade mode, wired into the install
  model's `mode`.

Also derive `PartialEq`/`Eq` for `station_api::CanisterInstallMode` to
support the CLI's full-mode verification.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XUVZ6KLc99kH1SGLJajJd8
Allow clippy::unwrap_used in the test module (the crate denies it at the
root) and avoid a redundant local rebinding flagged by
clippy::redundant_locals.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XUVZ6KLc99kH1SGLJajJd8
@aterga aterga requested a review from Copilot July 1, 2026 15:13
@aterga aterga marked this pull request as ready for review July 1, 2026 15:14
@aterga aterga requested a review from a team as a code owner July 1, 2026 15:14
@zeropath-ai

zeropath-ai Bot commented Jul 1, 2026

Copy link
Copy Markdown

No security or compliance issues detected. Reviewed everything up to 3cbcdd5.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► apps/wallet/src/components/external-canisters/CanisterInstallForm.spec.ts
    Add tests for upgrade mode visibility and emitted upgrade options
► apps/wallet/src/components/external-canisters/CanisterInstallForm.vue
    Add upgrade-mode UI, WasmMemoryPersistence and skip_pre_upgrade UI, and related logic
► apps/wallet/src/components/external-canisters/external-canisters.types.ts
    Introduce CanisterUpgradeOptions and WasmMemoryPersistence types
► apps/wallet/src/components/inputs/CanisterWasmMemoryPersistenceSelect.spec.ts
    Add tests for CanisterWasmMemoryPersistenceSelect
► apps/wallet/src/components/inputs/CanisterWasmMemoryPersistenceSelect.vue
    Implement new select component for wasm memory persistence
► apps/wallet/src/locales/en.locale.ts
    Add localization keys for wasm_memory_persistence and skip_pre_upgrade
► apps/wallet/src/locales/fr.locale.ts
    Add localization keys for wasm_memory_persistence and skip_pre_upgrade (French)
► apps/wallet/src/locales/pt.locale.ts
    Add localization keys for wasm_memory_persistence and skip_pre_upgrade (Portuguese)
► core/station/api/src/common.rs
    Derive PartialEq and Eq for CanisterInstallMode
► tests/integration/src/dfx_orbit/install.rs
    Update imports and add tests scaffolding for upgrade options and custom wasm sections
► tools/dfx-orbit/src/canister.rs
    Expose WasmMemoryPersistenceArgs and related types in public API
► tools/dfx-orbit/src/canister/install.rs
    Implement install_mode folding upgrade flags into upgrade options, add validation and tests for upgrade flags
► tools/dfx-orbit/README.md
    Document wasm-memory-persistence and skip-pre-upgrade usage for upgrades
► tools/dfx-orbit/src/canister/install.rs
    Add wasm_memory_persistence and skip_pre_upgrade fields to RequestCanisterInstallArgs, implement validation and upgrade option folding
► tools/dfx-orbit/src/canister.rs
    Add WasmMemoryPersistenceArgs enum and conversion to WasmMemoryPersistence
► tools/dfx-orbit/src/canister/install.rs
    Extend argument parsing and mode resolution to support upgrade options, and tests for upgrade scenarios

@aterga aterga enabled auto-merge (squash) July 1, 2026 15:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Wires the station’s existing upgrade-only options (wasm_memory_persistence, skip_pre_upgrade) through the two main user entry points—dfx-orbit CLI and the wallet’s external-canister Install screen—so upgrades can preserve Motoko EOP main memory and optionally bypass trapping pre_upgrade hooks.

Changes:

  • dfx-orbit: adds --wasm-memory-persistence <keep|replace> and --skip-pre-upgrade, folds them into CanisterInstallMode::Upgrade(Some(..)), rejects them for install/reinstall, and verifies the full mode (including upgrade options).
  • Wallet UI: adds an upgrade-only select + toggle, collapses back to plain { upgrade: [] } when unset, and adds i18n strings.
  • API: derives PartialEq/Eq for CanisterInstallMode to support full-mode verification comparisons.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tools/dfx-orbit/src/canister/install.rs Adds CLI flags, folds them into upgrade mode, improves verification to compare full mode, and prints upgrade options in request display; includes unit tests.
tools/dfx-orbit/src/canister.rs Re-exports the new CLI enum for wasm memory persistence.
tools/dfx-orbit/README.md Documents new CLI flags and usage for preserving main memory on upgrade.
tests/integration/src/dfx_orbit/install.rs Adds an integration test covering upgrade-options round-trip + verifier mismatch rejection.
core/station/api/src/common.rs Enables PartialEq/Eq on CanisterInstallMode (for CLI full-mode comparisons).
apps/wallet/src/locales/pt.locale.ts Adds Portuguese strings for the new upgrade options UI.
apps/wallet/src/locales/fr.locale.ts Adds French strings for the new upgrade options UI.
apps/wallet/src/locales/en.locale.ts Adds English strings for the new upgrade options UI.
apps/wallet/src/components/inputs/CanisterWasmMemoryPersistenceSelect.vue New select input mapping UI choice to candid-style union values.
apps/wallet/src/components/external-canisters/external-canisters.types.ts Adds derived TS helper types for upgrade options + wasm memory persistence.
apps/wallet/src/components/external-canisters/CanisterInstallForm.vue Adds upgrade-only persistence select + skip-pre-upgrade toggle; merges/collapses upgrade options into mode.
apps/wallet/src/components/external-canisters/CanisterInstallForm.spec.ts Extends form tests for the new UI behavior (with some missing coverage addressed in review comments).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…apping

Address review feedback by adding a dedicated spec for
CanisterWasmMemoryPersistenceSelect (select option <-> candid union
mapping) and tests asserting the skip_pre_upgrade toggle folds into
mode.upgrade[0].skip_pre_upgrade and collapses back to { upgrade: [] }
when cleared.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XUVZ6KLc99kH1SGLJajJd8

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, I really can't properly review the TS stuff. Nevertheless, I muddled through that portion of the review as best as I can, since reviewers are scarce these days...

Comment thread tools/dfx-orbit/src/canister/install.rs Outdated
Comment thread tools/dfx-orbit/src/canister/install.rs Outdated
Comment thread tools/dfx-orbit/src/canister/install.rs Outdated
Comment thread tools/dfx-orbit/src/canister/install.rs
Comment thread tools/dfx-orbit/src/canister/install.rs Outdated
Comment on lines +294 to +297
let persistence = match persistence {
WasmMemoryPersistence::Keep => "keep",
WasmMemoryPersistence::Replace => "replace",
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this just uses the same pattern as existing code (so just keep on doing this), but it was not a good pattern to begin with. What we are doing here is converting to a string. Such functionality is so fundamental that has optimized it to be LESS THAN 1 line: #[derive(Debug)]. Let's use that, as god intended:

writeln!(output, "Wasm memory persistence: {persistence:?}");

The only "problem" with this is that instead of "keep", you get "Keep". If you really hate the capital "K", you can get rid of it very concisely as follows:

let persistence = format!("{:?}", persistence).to_lowercase();
writeln!(output, "Wasm memory persistence: {persistence}");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b88cd2e — the display string is now derived via format!("{persistence:?}").to_lowercase(), as suggested.


Generated by Claude Code

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but how about mode? That's why I said,

I know that this just uses the same pattern as existing code (so just keep on doing this)

This thread was more of a rant than an actual suggestion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. For mode specifically, the Debug trick doesn't quite work: the Upgrade variant carries a payload, so {mode:?} would print Upgrade(Some(CanisterUpgradeOptionsInput { .. })) rather than a clean Upgrade — hence the hand-written match stays there.


Generated by Claude Code

Comment thread tests/integration/src/dfx_orbit/install.rs
Comment thread tests/integration/src/dfx_orbit/install.rs Outdated
Comment thread tests/integration/src/dfx_orbit/install.rs Outdated
Comment on lines +259 to +261
// The upgrade request is still pending, so the canister remains empty.
let status = canister_status(&env, Some(canister_ids.station), test_canister);
assert_eq!(status.module_hash, None);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense to follow through, i.e. install the wasm? I mean, ultimately, what we really want to see is that Keep and skip_pre_upgrade were actually, you know, PERFORMED as requested, right??

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b88cd2e — the test now installs the module first, then the approved upgrade executes with wasm_memory_persistence = replace + skip_pre_upgrade and wait_for_request asserts it completed, so the options are exercised through the management canister rather than left pending. One caveat: it uses replace instead of keep, because the test canister isn't built with Enhanced Orthogonal Persistence and the IC rejects keep for modules without the EOP metadata section. Truly proving keep preserves memory would need a Motoko EOP fixture wasm, which felt out of scope here.


Generated by Claude Code

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of scope here.

I don't think so. The whole motivation here is so that I can install a canister that was recently written in Motoko (and therefore, uses EOP).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point — done in 595ddfd. New integration test canister_upgrade_with_wasm_memory_persistence_keep: it marks the test module as EOP by appending the icp:private enhanced-orthogonal-persistence custom section (which is exactly how the IC detects EOP support — no Motoko toolchain needed), installs it, and then shows the same upgrade request fails without keep (the IC's safety check refuses to drop an EOP canister's main memory) and executes successfully with keep. That exercises the real motivating scenario end-to-end through the station.


Generated by Claude Code

emit('update:modelValue', { replace: null });
break;
default:
emit('update:modelValue', undefined);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like sweeping a problem under the rug.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b88cd2e — the catch-all default: arm is gone; 'default' is now an explicit case with a comment explaining it means "let the IC decide" and maps to an absent wasm_memory_persistence option in the request. The select's items are constrained to the three options, so every case is now spelled out.


Generated by Claude Code

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the IC decide automatically?? If so, then, it seems that this feature is not super necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No — the IC never infers keep. What it does is fail closed: upgrading a canister whose module declares EOP without keep is rejected outright (so no silent memory loss, but also no upgrade). It's dfx that infers client-side from the Wasm metadata and fills the flag in for you. So the option is necessary — without it, EOP canisters simply cannot be upgraded through Orbit at all. The new integration test in 595ddfd demonstrates both halves (rejected without keep, succeeds with it).


Generated by Claude Code

- Name conversion targets explicitly (WasmMemoryPersistence::from over
  Into::into) and replace .then_some with a plain if/else.
- Rename ensure_no_upgrade_options to err_if_upgrade_flags_were_passed.
- Derive the display string for the persistence mode via Debug instead of
  a hand-written match.
- Split the combined `pub use self::{..}` re-export into per-module
  statements.
- Fix the Enhanced Orthogonal Persistence docs link (docs moved to
  docs.internetcomputer.org).
- Integration test now installs the module first and executes the
  approved upgrade with wasm_memory_persistence=replace +
  skip_pre_upgrade, proving the options are forwarded to the IC (keep
  cannot execute against the non-EOP test wasm).
- Noun-y test variable names, assert the exact rejection error message,
  and fold the redundant plain-mode mapping tests into one.
- Make the "default" option mapping explicit in the persistence select
  instead of a catch-all switch arm.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XUVZ6KLc99kH1SGLJajJd8
claude added 2 commits July 3, 2026 13:03
…dule

Address review feedback: the motivating use case is upgrading Motoko
canisters with Enhanced Orthogonal Persistence, so exercise exactly that.
The new integration test marks the test module as EOP by appending the
`icp:private enhanced-orthogonal-persistence` custom section (which is
how the IC detects EOP support), installs it, and then shows the same
upgrade request fails without `keep` (the IC's safety check against
accidentally dropping main memory) and executes successfully with it.

Also link the README to the docs section on IC main memory retention and
correct the wording: the IC refuses to upgrade EOP canisters without
`keep` rather than silently clearing their memory.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XUVZ6KLc99kH1SGLJajJd8
…P section

The bundled test module is gzipped, and the IC reads the uncompressed
size of gzipped modules from the trailing bytes of the gzip stream — so
appending the custom section after the stream made the IC read the tail
of the section name ("ence") as a 1.7 GB module size. Decompress first;
the raw module with the appended section installs fine.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XUVZ6KLc99kH1SGLJajJd8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants